Skip to content
This repository has been archived by the owner on Aug 3, 2023. It is now read-only.

Allow worker with no routes to be published #2024

Merged
merged 8 commits into from
Aug 27, 2021
Merged

Conversation

jspspike
Copy link
Contributor

Fixes #1700

@jspspike jspspike requested a review from a team August 10, 2021 18:28
@jspspike jspspike marked this pull request as draft August 12, 2021 19:52
@jspspike
Copy link
Contributor Author

jspspike commented Aug 16, 2021

Confused if the behavior of when_top_level_zoneless_env_zoned_single_route_empty is actually intended. It essentially has workers_dev = true in the top level of the toml but then uses an environment where workers_dev is not set. Should the environment inherit the top level workers_dev and then create a zoneless (workers.dev) route or create a zoned route since the environment doesn't have wrangler_dev set?

@nilslice
Copy link
Contributor

@jspspike - looking at this now. An aside, but relevant to your PR... would you add a commit that updates this error message:

⚠️ No depolyment routes specified, worker will not be triggered. Please specify your deployment routes or set wrangler_dev = true inside of your configuration file in order to trigger your worker. For more information, see: https://developers.cloudflare.com/workers/cli-wrangler/configuration#keys

So that instead of wrangler_dev = true it is corrected to print workers_dev = true?

@nilslice
Copy link
Contributor

https://github.com/cloudflare/wrangler/blob/4e18717d7c632335db8198252d311c40d95d67d8/src/settings/toml/tests/deployments.rs#L808-L828

The test you mention (embedded above) generates a wrangler.toml manifest with the following configuration:

name = "top_level_zoneless_env_zoned_single_route_empty"
type = "webpack"
account_id = "fakeaccountid"
workers_dev = true

[env.test]
route = ""
zone_id = "samplezoneid"

Looking at this, I would assume the intention of these settings is to have no Zoned route, since there is no route defined despite the zone_id being present, but to inherit the workers_dev key into the environment. I'm not sure how the script naming works there (maybe append -test to the value in name?).

I assume this should be inherited based on the way we document the configuration, seen here:
image

Maybe @Electroid could comment on this assumption - and if we're in agreement, I think the test might need to be changed to expect 1 zoneless route (the workers.dev test route)? This would also mean some code is changed somewhere to inherit this top-level workers_dev key.

@jspspike jspspike marked this pull request as ready for review August 18, 2021 20:09
@jspspike
Copy link
Contributor Author

Going off of the docs linked I made workers_dev get passed down to environments. This made some tests need to be changed where both zoned and zoneless routes would be created if the environment got workers_dev = true from the top level and also had a zone/routes in the environment.

I went through the changed tests and tried to verify that the behavior is intended, but any changed tests should be looked at closely by the reviewer to make sure some intended behavior isn't changed since this will affect a lot of things in terms of what routes are deployed on publish

}
};

eprintln!("{:#?}", &wrangler_toml);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debug statement?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes to print the toml during tests, all the other toml helper functions had them except this one for some reason

@threepointone
Copy link
Contributor

I tested this with some variations and it all looked fine!

Copy link
Contributor

@threepointone threepointone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving because I tested the code and it looked good!

@Electroid Electroid self-requested a review August 27, 2021 15:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

publish worker without any routes or triggers
5 participants